-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Show Error Play Services Dialog when appropriate #491
Conversation
@lognaturel @yanokwa Please review and suggest changes |
Nice! When I run this with API 22 and play services disabled, I get a nice popup with an 'enable' button which takes me to the settings. But when I enable and come back to Collect, I see a white screen. I have to go back and then launch the geo activity again to make progress. Ideally either the geo activity would be displayed or I'd be back at the widget screen with the button to launch it. I like that you introduced a class with static method to share behavior. I don't like the structure of the If I missed something and you can't split it up I would recommend a name like |
If my first paragraph doesn't quite make sense you may want to try it for yourself by having Play Services installed, disabling it through the apps setting and then trying to launch a geo activity. |
Yeah, the first paragraph makes sense. I think i should send the user to the screen from where he can launch GeoActivity again or look into displaying the GeoActivity right away. I agree with the naming convention to make the code more readable. Will split the method into two: one that shows the dialog and other that checks for play services. |
Now, the user would be redirected to the calling activity after they have enabled play services. Also, made the necessary change regarding splitting of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one tiny style change. This is great, I really like the structure you ended up with!
If you are interested, one thing you could do that would be very helpful is propose a new hierarchy for the geo activities so that there can be more code sharing between them. You could think about it and then file an issue so we can come to agreement. For a good example, check out @dcbriccetti's #480 where he pulled out common behavior between activities that display lists. He also organized some of the code into smaller methods that make the code easier to read, made variables local when they could be and things like that.
|
||
resultCode = googleApiAvailability.isGooglePlayServicesAvailable(context); | ||
|
||
if (resultCode != ConnectionResult.SUCCESS) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just return resultCode == ConnectionResult.SUCCESS
here! If it's true, true is returned. If it's false, false is returned. "Boolean zen" 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, nice catch. Write less, Do more. 👍
Thanks, @Dvik! I recommend that you start creating branches for each project/feature/fix you work on. That way you can more easily work with a couple of different projects at a time if you want. It also makes it easier for other people to check out your code. |
Yeah,sure. Branches do help. Had a look at @dcbriccetti 's changes in the PR. Yes, we have to do something to better organize the Geo** activities. Will look into it and file a refactoring approach. |
#132 A request to location or map is now done only after checking if the play services version on the user's phone is available and is compatible with the play services version of the app.
Generalized the code keeping in mind DRY(Don't Repeat Yourself) by creating a PlayServicesUtils class with a static method which is called whenever needed.
API 16 without Google Play Services